Convert Animation Control to QML inspector section (v2.18.0) - #231
Merged
Conversation
…v2.18.0) - Replace AnimationControlWidget + AnimationControlSlider (QDockWidget) with AnimationControlController singleton + AnimationControlPanel.qml, integrated as a collapsible "Animation Control" section inside the Inspector panel - Vertical layout with typeahead dropdowns for animation and bone selection, styled keyframe fields matching TransformField (colored label + input + arrows), titled Translate / Scale / Orientation sections - Remove "Animation Control" toggle from View menu (now always visible in inspector) - Bump version to 2.18.0 - Replace animationcontrolwidget_test + animationcontrolslider_test with AnimationControlController_test covering singleton, tree, selection, navigation, add/delete keyframe, and value-setter paths Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughReplaces the legacy widget-based animation UI with a QML panel and a new C++ QML-accessible singleton controller. Adds AnimationControlController (header, impl, tests), a QML AnimationControlPanel, integrates it into PropertiesPanel, updates resources/CMake, removes old widget/slider code and tests, and registers/tears down the controller in MainWindow. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (QML)
participant Panel as AnimationControlPanel
participant Controller as AnimationControlController
participant Ogre as Ogre Engine
User->>Panel: choose animation / bone / edit value
Panel->>Controller: selectAnimation(entity, anim) / selectBone(bone) / setKf... / addKeyframe()/deleteKeyframe()
Controller->>Ogre: query animation/skeleton/tracks / set AnimationState time / modify keyframes
Ogre-->>Controller: animation data, bone list, keyframes
Controller->>Controller: build animationTree, compute slider range, find nearest keyframe
Controller-->>Panel: animationTreeChanged(), boneListChanged(), sliderMaximum, keyframeTicksChanged(), currentKeyframeChanged()
User->>Panel: move slider / set frame
Panel->>Controller: setSliderValue(ms) / setAnimationFrame(ms)
Controller->>Ogre: set AnimationState time
Controller-->>Panel: sliderValueChanged(), currentKeyframeChanged()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mainwindow.cpp (1)
158-233:⚠️ Potential issue | 🟠 MajorMissing
AnimationControlController::kill()in destructor may cause shutdown crashes.The
AnimationControlControllersingleton holds raw Ogre pointers (m_selectedEntity,m_selectedSkeleton,m_selectedTrack) and has an active poll timer. WhenManager::kill()is called at line 231, these Ogre objects are destroyed, but the controller's timer could still fire and access them.🐛 Proposed fix
Add
AnimationControlController::kill()beforeManager::kill():// Only destroy Manager if it still exists and belongs to this MainWindow // (In tests, Manager may be destroyed separately in TearDown) Manager* manager = Manager::getSingletonPtr(); if(manager && manager->getMainWindow() == this) { + // Destroy controller before Manager to prevent dangling Ogre pointers + AnimationControlController::kill(); // Only destroy if this MainWindow owns the Manager Manager::kill(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mainwindow.cpp` around lines 158 - 233, The destructor MainWindow::~MainWindow currently calls Manager::kill() after destroying widgets, but fails to stop the AnimationControlController which holds raw Ogre pointers and an active poll timer; add a call to AnimationControlController::kill() (or otherwise stop its timer and clear its Ogre pointers) immediately before Manager::kill() so the controller cannot access destroyed Ogre resources—locate the end of MainWindow::~MainWindow (the block that retrieves Manager::getSingletonPtr() and calls Manager::kill()) and insert AnimationControlController::kill() just prior to invoking Manager::kill().
🧹 Nitpick comments (2)
src/AnimationControlController.cpp (1)
255-263: Consider removing the empty conditional block.Lines 257-259 have an empty if-block with a comment. The condition
ms == m_sliderValue && m_selectedEntitydoes nothing when true. Either remove the check or clarify the intent with an actual action.♻️ Suggested simplification
void AnimationControlController::setSliderValue(int ms) { - if (ms == m_sliderValue && m_selectedEntity) { - // Still call setAnimationFrame to keep Ogre in sync on explicit user drags - } m_sliderValue = ms; emit sliderValueChanged(); setAnimationFrame(ms); }The
setAnimationFrame(ms)is called unconditionally anyway, so the comment's intent is already achieved.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AnimationControlController.cpp` around lines 255 - 263, The if-block in AnimationControlController::setSliderValue that checks (ms == m_sliderValue && m_selectedEntity) is empty and redundant because setAnimationFrame(ms) is called unconditionally; remove that empty conditional entirely (delete the if(...) { /* comment */ } block) so the method simply updates m_sliderValue, emits sliderValueChanged(), and calls setAnimationFrame(ms), keeping behavior unchanged and eliminating a no-op.src/mainwindow.cpp (1)
378-380: Consider using the new signal-slot connection syntax.The SIGNAL/SLOT macros don't provide compile-time checking. The new Qt5+ syntax catches connection errors at compile time.
♻️ Suggested modernization
- connect(pAnimationWidget, SIGNAL(changeAnimationName(const std::string&)), - AnimationControlController::instance(), SLOT(updateAnimationTree())); + connect(pAnimationWidget, &AnimationWidget::changeAnimationName, + AnimationControlController::instance(), &AnimationControlController::updateAnimationTree);Note: This requires
changeAnimationNamesignal signature to be compatible withupdateAnimationTree()slot (no arguments). IfchangeAnimationNamehas an argument, use a lambda to discard it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mainwindow.cpp` around lines 378 - 380, The connection uses old SIGNAL/SLOT macros which lack compile-time checking; replace it with the Qt5+ pointer-to-member syntax by connecting pAnimationWidget's changeAnimationName signal to the AnimationControlController::instance()->updateAnimationTree slot (use &AnimationWidget::changeAnimationName and &AnimationControlController::updateAnimationTree), and if changeAnimationName carries a parameter, use a lambda that accepts the argument and calls AnimationControlController::instance()->updateAnimationTree() discarding the parameter to match the no-arg slot.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@qml/AnimationControlPanel.qml`:
- Around line 51-53: Guard the parseFloat usage in the input handlers so we
never call kfRoot.committed with NaN: in the onEditingFinished, Keys.onUpPressed
and Keys.onDownPressed handlers (and the duplicate block at lines ~73-85) parse
the current text into a var v and only call kfRoot.committed(v) (and only update
text) when v is a valid number (e.g., check !isNaN(v) or isFinite(v)); if
parseFloat(text) yields NaN, do not modify text or call kfRoot.committed (or
optionally fall back to a defined base value before applying kfRoot.step).
- Around line 457-482: The tick overlay doesn't repaint when the
playhead/selection changes; add Connections handlers on
AnimationControlController to call tickCanvas.requestPaint() for
selection/playhead changes — specifically implement functions for
onSelectedTickChanged() and onPlayheadPositionChanged() (or the controller's
equivalent playhead change signal) alongside the existing
onKeyframeTicksChanged/onAnimationLengthChanged/onThemeChanged so the canvas
repaints whenever AnimationControlController.selectedTick or the playhead moves.
In `@src/AnimationControlController.cpp`:
- Around line 489-497: The KF_SET_ROT macro-generated setters
(AnimationControlController::setKfRot##AXIS) directly assign a single component
on the quaternion returned by m_currentKeyframe->getRotation(), which can break
the unit-length invariant; update these setters to normalize the quaternion
before calling m_currentKeyframe->setRotation(r) (or replace per-axis setters
with a combined setKfRotation that takes all components then normalizes),
ensuring you still update m_kfRot##AXIS and call notifyOgreUpdate(); apply the
change where KF_SET_ROT is defined so all setKfRotW/X/Y/Z use the normalization
step.
In `@src/animationcontrolslider_test.cpp`:
- Around line 1-2: Remove the obsolete widget implementation for
AnimationControlSlider: delete the files animationcontrolslider.h and
animationcontrolslider.cpp (or mark them deprecated and add a clear deprecation
comment/guard) because the UI was migrated to a QML Canvas overlay and the
slider behavior is covered by AnimationControlController_test.cpp; also search
the repo for any remaining references to the class/symbol AnimationControlSlider
and remove or update them (including build/CMake/PRO targets) so no leftover
link or compile references remain.
---
Outside diff comments:
In `@src/mainwindow.cpp`:
- Around line 158-233: The destructor MainWindow::~MainWindow currently calls
Manager::kill() after destroying widgets, but fails to stop the
AnimationControlController which holds raw Ogre pointers and an active poll
timer; add a call to AnimationControlController::kill() (or otherwise stop its
timer and clear its Ogre pointers) immediately before Manager::kill() so the
controller cannot access destroyed Ogre resources—locate the end of
MainWindow::~MainWindow (the block that retrieves Manager::getSingletonPtr() and
calls Manager::kill()) and insert AnimationControlController::kill() just prior
to invoking Manager::kill().
---
Nitpick comments:
In `@src/AnimationControlController.cpp`:
- Around line 255-263: The if-block in
AnimationControlController::setSliderValue that checks (ms == m_sliderValue &&
m_selectedEntity) is empty and redundant because setAnimationFrame(ms) is called
unconditionally; remove that empty conditional entirely (delete the if(...) { /*
comment */ } block) so the method simply updates m_sliderValue, emits
sliderValueChanged(), and calls setAnimationFrame(ms), keeping behavior
unchanged and eliminating a no-op.
In `@src/mainwindow.cpp`:
- Around line 378-380: The connection uses old SIGNAL/SLOT macros which lack
compile-time checking; replace it with the Qt5+ pointer-to-member syntax by
connecting pAnimationWidget's changeAnimationName signal to the
AnimationControlController::instance()->updateAnimationTree slot (use
&AnimationWidget::changeAnimationName and
&AnimationControlController::updateAnimationTree), and if changeAnimationName
carries a parameter, use a lambda that accepts the argument and calls
AnimationControlController::instance()->updateAnimationTree() discarding the
parameter to match the no-arg slot.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5adf50e9-e77a-4791-8e1e-97bd25057d96
📒 Files selected for processing (12)
CMakeLists.txtqml/AnimationControlPanel.qmlqml/PropertiesPanel.qmlsrc/AnimationControlController.cppsrc/AnimationControlController.hsrc/AnimationControlController_test.cppsrc/CMakeLists.txtsrc/animationcontrolslider_test.cppsrc/animationcontrolwidget_test.cppsrc/mainwindow.cppsrc/qml_resources.qrcui_files/mainwindow.ui
💤 Files with no reviewable changes (1)
- ui_files/mainwindow.ui
Comment on lines
+457
to
+482
| Canvas { | ||
| id: tickCanvas; anchors.fill: parent; enabled: false | ||
| onPaint: { | ||
| var ctx = getContext("2d"); ctx.clearRect(0, 0, width, height) | ||
| var maxMs = AnimationControlController.sliderMaximum; if (maxMs <= 0) return | ||
| var pad = 13; var avail = width - pad * 2 | ||
| var ticks = AnimationControlController.keyframeTicks; var selTk = AnimationControlController.selectedTick | ||
| for (var i = 0; i < ticks.length; i++) { | ||
| var x = pad + (ticks[i] / maxMs) * avail; var isSel = (ticks[i] === selTk) | ||
| if (isSel) { | ||
| ctx.strokeStyle = "#ff4444"; ctx.lineWidth = 3 | ||
| ctx.beginPath(); ctx.moveTo(x, 4); ctx.lineTo(x, height); ctx.stroke() | ||
| ctx.fillStyle = "#ff4444" | ||
| ctx.beginPath(); ctx.moveTo(x - 5, 2); ctx.lineTo(x + 5, 2); ctx.lineTo(x, 8); ctx.closePath(); ctx.fill() | ||
| } else { | ||
| ctx.strokeStyle = "#ffcc00"; ctx.lineWidth = 1.5 | ||
| ctx.beginPath(); ctx.moveTo(x, 2); ctx.lineTo(x, height - 2); ctx.stroke() | ||
| } | ||
| } | ||
| } | ||
| Connections { | ||
| target: AnimationControlController | ||
| function onKeyframeTicksChanged() { tickCanvas.requestPaint() } | ||
| function onAnimationLengthChanged() { tickCanvas.requestPaint() } | ||
| function onThemeChanged() { tickCanvas.requestPaint() } | ||
| } |
There was a problem hiding this comment.
Repaint the tick overlay when the playhead changes.
Lines 463-476 render selectedTick, but this canvas only repaints on tick-list, length, or theme changes. Scrubbing the slider or jumping between keyframes can change the selection without ever refreshing the overlay.
🖌️ Suggested fix
Connections {
target: AnimationControlController
function onKeyframeTicksChanged() { tickCanvas.requestPaint() }
+ function onSliderValueChanged() { tickCanvas.requestPaint() }
function onAnimationLengthChanged() { tickCanvas.requestPaint() }
function onThemeChanged() { tickCanvas.requestPaint() }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@qml/AnimationControlPanel.qml` around lines 457 - 482, The tick overlay
doesn't repaint when the playhead/selection changes; add Connections handlers on
AnimationControlController to call tickCanvas.requestPaint() for
selection/playhead changes — specifically implement functions for
onSelectedTickChanged() and onPlayheadPositionChanged() (or the controller's
equivalent playhead change signal) alongside the existing
onKeyframeTicksChanged/onAnimationLengthChanged/onThemeChanged so the canvas
repaints whenever AnimationControlController.selectedTick or the playhead moves.
Comment on lines
+489
to
+497
| #define KF_SET_ROT(AXIS, FIELD) \ | ||
| void AnimationControlController::setKfRot##AXIS(double v) { \ | ||
| if (m_updatingValues || !m_currentKeyframe) return; \ | ||
| Ogre::Quaternion r = m_currentKeyframe->getRotation(); \ | ||
| r.FIELD = static_cast<float>(v); \ | ||
| m_currentKeyframe->setRotation(r); \ | ||
| m_kfRot##AXIS = v; \ | ||
| notifyOgreUpdate(); \ | ||
| } |
There was a problem hiding this comment.
Quaternion component modification may produce non-unit quaternions.
Setting individual quaternion components (w, x, y, z) independently doesn't maintain the unit quaternion constraint. This could lead to rendering artifacts or undefined behavior in Ogre's animation interpolation.
Consider normalizing the quaternion after modification or providing a combined setter that maintains the constraint:
🛡️ Optional normalization fix
`#define` KF_SET_ROT(AXIS, FIELD) \
void AnimationControlController::setKfRot##AXIS(double v) { \
if (m_updatingValues || !m_currentKeyframe) return; \
Ogre::Quaternion r = m_currentKeyframe->getRotation(); \
r.FIELD = static_cast<float>(v); \
+ r.normalise(); \
m_currentKeyframe->setRotation(r); \
m_kfRot##AXIS = v; \
notifyOgreUpdate(); \
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #define KF_SET_ROT(AXIS, FIELD) \ | |
| void AnimationControlController::setKfRot##AXIS(double v) { \ | |
| if (m_updatingValues || !m_currentKeyframe) return; \ | |
| Ogre::Quaternion r = m_currentKeyframe->getRotation(); \ | |
| r.FIELD = static_cast<float>(v); \ | |
| m_currentKeyframe->setRotation(r); \ | |
| m_kfRot##AXIS = v; \ | |
| notifyOgreUpdate(); \ | |
| } | |
| `#define` KF_SET_ROT(AXIS, FIELD) \ | |
| void AnimationControlController::setKfRot##AXIS(double v) { \ | |
| if (m_updatingValues || !m_currentKeyframe) return; \ | |
| Ogre::Quaternion r = m_currentKeyframe->getRotation(); \ | |
| r.FIELD = static_cast<float>(v); \ | |
| r.normalise(); \ | |
| m_currentKeyframe->setRotation(r); \ | |
| m_kfRot##AXIS = v; \ | |
| notifyOgreUpdate(); \ | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/AnimationControlController.cpp` around lines 489 - 497, The KF_SET_ROT
macro-generated setters (AnimationControlController::setKfRot##AXIS) directly
assign a single component on the quaternion returned by
m_currentKeyframe->getRotation(), which can break the unit-length invariant;
update these setters to normalize the quaternion before calling
m_currentKeyframe->setRotation(r) (or replace per-axis setters with a combined
setKfRotation that takes all components then normalizes), ensuring you still
update m_kfRot##AXIS and call notifyOgreUpdate(); apply the change where
KF_SET_ROT is defined so all setKfRotW/X/Y/Z use the normalization step.
Comment on lines
+1
to
+2
| // AnimationControlSlider has been removed; the timeline is now a QML Canvas overlay. | ||
| // The slider tick functionality is covered indirectly by AnimationControlController_test.cpp. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if animationcontrolslider files still exist and if they're referenced anywhere
echo "=== Checking if slider files exist ==="
fd -t f 'animationcontrolslider' --extension h --extension cpp
echo "=== Checking for references to AnimationControlSlider ==="
rg -l 'AnimationControlSlider' --type cpp --type h -g '!*_test.cpp' || echo "No references found"Repository: fernandotonon/QtMeshEditor
Length of output: 322
Remove AnimationControlSlider source files or mark them as deprecated.
The animationcontrolslider.h and animationcontrolslider.cpp files still exist in the codebase but are not referenced by any non-test code. Since the test file indicates the slider has been replaced by a QML Canvas overlay and the coding guidelines require new UI to be built in QML, these obsolete widget files should be removed from the repository to avoid confusion and maintain a clean codebase.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/animationcontrolslider_test.cpp` around lines 1 - 2, Remove the obsolete
widget implementation for AnimationControlSlider: delete the files
animationcontrolslider.h and animationcontrolslider.cpp (or mark them deprecated
and add a clear deprecation comment/guard) because the UI was migrated to a QML
Canvas overlay and the slider behavior is covered by
AnimationControlController_test.cpp; also search the repo for any remaining
references to the class/symbol AnimationControlSlider and remove or update them
(including build/CMake/PRO targets) so no leftover link or compile references
remain.
…MakeLists.txt Swap animationcontrolwidget.cpp/.h and animationcontrolslider.cpp/.h for AnimationControlController.cpp/.h so per-test executables that include mainwindow.cpp can resolve AnimationControlController symbols. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…dress Comparing pointers after kill()+instance() is unreliable — the allocator may reuse the same address. Instead mutate state, kill, and assert the new instance starts with default values. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/AnimationControlController_test.cpp (1)
20-20: Use event-driven waits instead of fixed sleeps in timer tests to improve Xvfb compatibility.Fixed-duration
QThread::msleep()calls (lines 482–501) make these timer tests unreliable under headless/loaded CI. Wait on observable state (e.g.,QSignalSpyon the poll timer's signal or Qt'sQTRY_*macros with bounded timeout) instead. The codebase already demonstrates this pattern at lines 124 and 177.Line 20's cleanup sleep in
SetUp()is acceptable for ensuring singleton teardown.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AnimationControlController_test.cpp` at line 20, Replace the fixed-duration QThread::msleep() calls in the timer tests of AnimationControlController_test.cpp (the sleeps around the poll timer checks, not the SetUp() cleanup sleep) with event-driven waits: use QSignalSpy attached to the poll timer's timeout signal (or other observable signal emitted by the component under test) and wait for the expected signal count, or use Qt's QTRY_* macros (e.g., QTRY_COMPARE/QTRY_VERIFY with a bounded timeout) to assert state changes; keep the small QThread::msleep() in SetUp() for singleton teardown as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/AnimationControlController_test.cpp`:
- Line 228: Add a precondition assert to guard each call that uses the first
bone: insert ASSERT_FALSE(ctrl->boneNames().isEmpty()); immediately before any
statement like ctrl->selectBone(ctrl->boneNames().first()); to ensure the bone
list is non-empty and yield a clear test failure; update all test sites that
invoke ctrl->boneNames().first() (e.g., the calls to selectBone using
ctrl->boneNames().first()) so they first check
ASSERT_FALSE(ctrl->boneNames().isEmpty());.
---
Nitpick comments:
In `@src/AnimationControlController_test.cpp`:
- Line 20: Replace the fixed-duration QThread::msleep() calls in the timer tests
of AnimationControlController_test.cpp (the sleeps around the poll timer checks,
not the SetUp() cleanup sleep) with event-driven waits: use QSignalSpy attached
to the poll timer's timeout signal (or other observable signal emitted by the
component under test) and wait for the expected signal count, or use Qt's QTRY_*
macros (e.g., QTRY_COMPARE/QTRY_VERIFY with a bounded timeout) to assert state
changes; keep the small QThread::msleep() in SetUp() for singleton teardown
as-is.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 89afa3fa-8934-4e41-ae45-bc0ef6bb7f1a
📒 Files selected for processing (1)
src/AnimationControlController_test.cpp
…s, test fixes - mainwindow.cpp: kill AnimationControlController before Manager::kill() to prevent the poll timer from accessing dangling Ogre pointers at shutdown - AnimationControlPanel.qml: guard arrow-button handlers against NaN input; repaint tick canvas on sliderValueChanged so selected-tick indicator updates while scrubbing the timeline - AnimationControlController_test.cpp: add ASSERT_FALSE(boneNames().isEmpty()) before every boneNames().first() call; replace QThread::msleep+processEvents in poll-timer tests with QTest::qWait for event-driven waiting; add <QTest> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/mainwindow.cpp (1)
380-382: Consider modernizing to Qt5 signal/slot syntax.The old-style
SIGNAL/SLOTmacros work but lose compile-time type checking. The modern syntax catches mismatches at compile time:- connect(pAnimationWidget, SIGNAL(changeAnimationName(const std::string&)), - AnimationControlController::instance(), SLOT(updateAnimationTree())); + connect(pAnimationWidget, &AnimationWidget::changeAnimationName, + AnimationControlController::instance(), [](const std::string&) { + AnimationControlController::instance()->updateAnimationTree(); + });Alternatively, if you prefer not to capture the unused parameter:
connect(pAnimationWidget, &AnimationWidget::changeAnimationName, AnimationControlController::instance(), &AnimationControlController::updateAnimationTree);(Qt allows connecting signals with extra parameters to slots with fewer parameters in both syntaxes.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mainwindow.cpp` around lines 380 - 382, Replace the old SIGNAL/SLOT macros for the pAnimationWidget connection with the Qt5 function-pointer syntax: connect the AnimationWidget::changeAnimationName signal to AnimationControlController::updateAnimationTree using the modern pointer-to-member form (use AnimationWidget::changeAnimationName and AnimationControlController::updateAnimationTree and AnimationControlController::instance() as the receiver); you can connect the signal with an extra parameter to a slot with no parameters so no lambda capture is required if the parameter is unused.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@qml/AnimationControlPanel.qml`:
- Around line 52-53: Compute and guard the parsed base value in the keyboard
handlers: in Keys.onUpPressed and Keys.onDownPressed first assign var base =
parseFloat(text); then only perform the step/update and call kfRoot.committed if
kfRoot.editable && !isNaN(base); keep using kfRoot.step, kfRoot.decimals and
kfRoot.committed as before so you avoid committing NaN values.
---
Nitpick comments:
In `@src/mainwindow.cpp`:
- Around line 380-382: Replace the old SIGNAL/SLOT macros for the
pAnimationWidget connection with the Qt5 function-pointer syntax: connect the
AnimationWidget::changeAnimationName signal to
AnimationControlController::updateAnimationTree using the modern
pointer-to-member form (use AnimationWidget::changeAnimationName and
AnimationControlController::updateAnimationTree and
AnimationControlController::instance() as the receiver); you can connect the
signal with an extra parameter to a slot with no parameters so no lambda capture
is required if the parameter is unused.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 333cdb56-5fd2-46fc-9a9c-471debbb805e
📒 Files selected for processing (3)
qml/AnimationControlPanel.qmlsrc/AnimationControlController_test.cppsrc/mainwindow.cpp
Comment on lines
+52
to
+53
| Keys.onUpPressed: { if (kfRoot.editable) { var v = parseFloat(text) + kfRoot.step; text = v.toFixed(kfRoot.decimals); kfRoot.committed(v) } } | ||
| Keys.onDownPressed: { if (kfRoot.editable) { var v = parseFloat(text) - kfRoot.step; text = v.toFixed(kfRoot.decimals); kfRoot.committed(v) } } |
There was a problem hiding this comment.
Keyboard step handlers still lack NaN guard.
The arrow button click handlers (lines 74, 85) were fixed to check !isNaN(base), but the keyboard handlers still commit potentially invalid values:
Keys.onUpPressed: { if (kfRoot.editable) { var v = parseFloat(text) + kfRoot.step; ... kfRoot.committed(v) } }
Keys.onDownPressed: { if (kfRoot.editable) { var v = parseFloat(text) - kfRoot.step; ... kfRoot.committed(v) } }If the field is empty or mid-edit, parseFloat(text) returns NaN, and NaN + step remains NaN.
🛡️ Proposed fix to add NaN guard
- Keys.onUpPressed: { if (kfRoot.editable) { var v = parseFloat(text) + kfRoot.step; text = v.toFixed(kfRoot.decimals); kfRoot.committed(v) } }
- Keys.onDownPressed: { if (kfRoot.editable) { var v = parseFloat(text) - kfRoot.step; text = v.toFixed(kfRoot.decimals); kfRoot.committed(v) } }
+ Keys.onUpPressed: { if (kfRoot.editable) { var base = parseFloat(text); if (!isNaN(base)) { var v = base + kfRoot.step; text = v.toFixed(kfRoot.decimals); kfRoot.committed(v) } } }
+ Keys.onDownPressed: { if (kfRoot.editable) { var base = parseFloat(text); if (!isNaN(base)) { var v = base - kfRoot.step; text = v.toFixed(kfRoot.decimals); kfRoot.committed(v) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@qml/AnimationControlPanel.qml` around lines 52 - 53, Compute and guard the
parsed base value in the keyboard handlers: in Keys.onUpPressed and
Keys.onDownPressed first assign var base = parseFloat(text); then only perform
the step/update and call kfRoot.committed if kfRoot.editable && !isNaN(base);
keep using kfRoot.step, kfRoot.decimals and kfRoot.committed as before so you
avoid committing NaN values.
|
This was referenced May 1, 2026
This was referenced Jun 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
AnimationControlWidget(bottom dock) andAnimationControlSliderare replaced byAnimationControlController(QML singleton) +AnimationControlPanel.qml, embedded as a collapsible "Animation Control" section inside the existing Inspector panel — no extra dock window neededTransformFieldwith colored labels and up/down arrows; sections titled Translate / Scale / OrientationsectionVisiblebindinganimationcontrolwidget_testandanimationcontrolslider_testwithAnimationControlController_testcovering singleton lifecycle, animation tree population, selection, keyframe navigation, add/delete, and value-setter pathsTest plan
|</>|navigate between keyframes; tick marks update+KFadds a keyframe at current position;-KFremoves it🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Tests
Chores